Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update 'Add credentials' dropdown button #551

Merged
merged 9 commits into from
Oct 6, 2024

Conversation

janfaracik
Copy link
Contributor

@janfaracik janfaracik commented Jul 15, 2024

This PR updates the 'Add credentials' button to use the .jenkins-button class as well as to use the Dropdown component over the legacy YUI dropdown.

Before
image

After
image

Testing done

Submitter checklist

  • Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
  • Ensure that the pull request title represents the desired changelog entry
  • Please describe what you did
  • Link to relevant issues in GitHub or Jira
  • Link to relevant pull requests, esp. upstream and downstream changes
  • Ensure you have provided tests - that demonstrates feature works or fixes the issue

@mawinter69 mawinter69 mentioned this pull request Aug 2, 2024
6 tasks
@janfaracik janfaracik marked this pull request as ready for review September 14, 2024 08:47
@janfaracik janfaracik requested a review from a team as a code owner September 14, 2024 08:47
@jtnord
Copy link
Member

jtnord commented Sep 27, 2024

Has compatability with the ATH been checked with this? The code change otherwise looks reasonable to me.

basil added a commit to basil/acceptance-test-harness that referenced this pull request Oct 2, 2024
basil added a commit to basil/bom that referenced this pull request Oct 2, 2024
@basil basil removed the request for review from a team October 2, 2024 00:26
basil added a commit to basil/acceptance-test-harness that referenced this pull request Oct 2, 2024
basil added a commit to basil/bom that referenced this pull request Oct 2, 2024
basil
basil previously approved these changes Oct 2, 2024
Copy link
Member

@basil basil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@basil basil requested a review from a team October 2, 2024 01:32
@basil basil dismissed their stale review October 2, 2024 02:19

Not ready yet

@basil
Copy link
Member

basil commented Oct 2, 2024

@janfaracik This results in 36 new ATH failures in jenkinsci/acceptance-test-harness#1752. Can you prepare an ATH PR that adapts to this change?

@janfaracik
Copy link
Contributor Author

@janfaracik This results in 36 new ATH failures in jenkinsci/acceptance-test-harness#1752. Can you prepare an ATH PR that adapts to this change?

Thanks Basil! I will do :)

@janfaracik
Copy link
Contributor Author

@basil Heya, when updating ATH tests for plugin changes, do we need to maintain compatibility with the old version of the plugin?

basil pushed a commit to jenkinsci/acceptance-test-harness that referenced this pull request Oct 6, 2024
basil added a commit to basil/acceptance-test-harness that referenced this pull request Oct 6, 2024
@timja
Copy link
Member

timja commented Oct 6, 2024

@basil Heya, when updating ATH tests for plugin changes, do we need to maintain compatibility with the old version of the plugin?

FWIW I know Basil asked you to maintain forward compatibility it depends on whether the plugin baseline is supported on the latest LTS version really.

If the change will be released and it will work on latest weekly and latest LTS then you don't really need forward compatibility as we only support the latest version of the plugin in ATH, but if its easy to maintain the compatibility then it makes it so you can merge the ATH change independently which is a good thing

@basil basil merged commit f0a2ed0 into jenkinsci:master Oct 6, 2024
18 checks passed
@janfaracik janfaracik deleted the update-dropdown-button branch October 7, 2024 08:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants